Fix table listing in Iceberg to skip non-Iceberg tables #1354

lxynov · 2019-08-21T23:48:21Z

#1324

Tests done:

mvn clean install builds successfully in presto-hive directory.
Introduced TestIcebergMetadataListing. mvn clean install builds successfully in presto-iceberg directory.
End-to-end tests look good.

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

findepi · 2019-08-22T09:24:52Z

presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java

@@ -176,7 +178,7 @@ public ConnectorTableMetadata getTableMetadata(ConnectorSession session, Connect
        return schemaName.map(Collections::singletonList)
                .orElseGet(metastore::getAllDatabases)
                .stream()
-                .flatMap(schema -> metastore.getAllTables(schema).stream()
+                .flatMap(schema -> metastore.getTablesMatchingParameter(schema, TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE).stream()


Is the match case-sensitive or case-insensitive?

On our side we do case-insensitive, file metastore it's case-sensitive and HMS thrift call may be case-sensitive (or it may actually depend on hms backend db collation).

@electrum thoughts?

@findepi @linxingyuan1102 The change introduces the problematic behavior as AFAIK case-sensitivity depends on HMS backend db collation and for derby db it is case-sensitive. This leads to iceberg tables that are created in spark using an upper case 'ICEBERG' type become hidden in show tables while still valid and usable in queries.

@vrozov thanks for your comment. As you see, this was assumed as probable.
May I suggest to move discussion to #iceberg slack channel?
Otherwise a conversation under a closed PR may escape attention of some people.

@findepi Both #iceberg slack channel or new issue #1592 is fine with me. I commented here, to make it easier to refer to the root cause of the problem.

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java

lxynov · 2019-08-23T21:56:00Z

@findepi @sopel39
Thanks for the review. I've updated the PR.

sopel39

LGTM form me. Let's wait for @findepi review

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java

lxynov · 2019-08-26T21:08:01Z

@findepi Thanks for the review. Comments addressed.

findepi

Looks good to me!

Minor comments + a question to David.

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

...to-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java

lxynov · 2019-08-28T01:15:41Z

Thanks. Comments addressed.

findepi

LGTM

@linxingyuan1102 thanks!

@electrum do you want to review changes in FileHiveMetastore.java?

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

phd3 · 2019-08-30T01:09:11Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

@@ -119,6 +120,9 @@
    private volatile boolean metastoreKnownToSupportTableParamEqualsPredicate;
    private volatile boolean metastoreKnownToSupportTableParamLikePredicate;

+    private static final Pattern TABLE_PARAMETER_SAFE_KEY_PATTERN = Pattern.compile("^[a-zA-Z_]+$");


Is these a reference to what are the safe values for HMS?

I don't think there is one. See #1354 (comment).

@phd3 this is documented in the code (where the constant is actually used, so there is a little bit more context).
Could you please check that explanation there? If it's not sufficient for a reader, we should improve it.

phd3 · 2019-08-30T01:20:53Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java

@@ -86,6 +87,7 @@
    private final LoadingCache<String, List<String>> databaseNamesCache;
    private final LoadingCache<HiveTableName, Optional<Table>> tableCache;
    private final LoadingCache<String, List<String>> tableNamesCache;
+    private final LoadingCache<List<String>, List<String>> tablesWithParameterCache;


I'd rather create a different class for <databaseName, parameterKey, parameterValue> than using a list to key the cache.

nit: we can name it something liketableNamesCacheForPattern for consistency with tableNamesCache. both of them fetch tables for a database, the later one takes a filter into account.

nit: we can name it something liketableNamesCacheForPattern for consistency with tableNamesCache. both of them fetch tables for a database, the later one takes a filter into account.

"Pattern" might not be very accurate because we use both = and LIKE when constructing the filter and we restrict the filter value to be alphanumeric. And I think it should be better if we just name the cache field after the method whose results it is caching. So I'd rather stay with tablesWithParameterCache here.

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java

electrum

A few minor comments, otherwise looks good

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

...to-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java

presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java

lxynov · 2019-09-03T23:03:46Z

@phd3 @findepi @electrum
Thanks for the review. I've addressed the comments.

lxynov · 2019-09-10T00:38:56Z

I've rebased the changes against master to resolve conflicts. Do we want to merge this if everything is okay?

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java

findepi

LGTM, I will merge when Travis passes
(please ping me if I don't)

...to-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

electrum

Thanks for splitting this up into multiple commits. Very easy to review and see the different changes.

This is mainly used in tests.

Table listing in the Iceberg catalog should not include non-Iceberg tables.

findepi · 2019-09-11T07:23:40Z

Merged, thanks!

cla-bot bot added the cla-signed label Aug 21, 2019

findepi reviewed Aug 22, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java Outdated Show resolved Hide resolved

sopel39 reviewed Aug 22, 2019

View reviewed changes

findepi requested changes Aug 22, 2019

View reviewed changes

findepi reviewed Aug 22, 2019

View reviewed changes

lxynov force-pushed the iceberg-table-listing branch from c67062e to 3685028 Compare August 23, 2019 21:53

sopel39 reviewed Aug 26, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java Outdated Show resolved Hide resolved

findepi reviewed Aug 26, 2019

View reviewed changes

presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java Outdated Show resolved Hide resolved

presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java Outdated Show resolved Hide resolved

lxynov force-pushed the iceberg-table-listing branch from 3685028 to 2ef434b Compare August 26, 2019 21:05

findepi approved these changes Aug 27, 2019

View reviewed changes

lxynov force-pushed the iceberg-table-listing branch from 2ef434b to e412b9f Compare August 28, 2019 01:12

findepi approved these changes Aug 28, 2019

View reviewed changes

phd3 reviewed Aug 30, 2019

View reviewed changes

findepi assigned electrum Aug 31, 2019

electrum approved these changes Sep 2, 2019

View reviewed changes

lxynov force-pushed the iceberg-table-listing branch 2 times, most recently from 37ee2dc to f5185b6 Compare September 3, 2019 22:31

lxynov force-pushed the iceberg-table-listing branch from 214e4f4 to a086e9d Compare September 10, 2019 00:24

findepi reviewed Sep 10, 2019

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java Outdated Show resolved Hide resolved

lxynov force-pushed the iceberg-table-listing branch from a086e9d to 495770f Compare September 10, 2019 17:22

findepi approved these changes Sep 10, 2019

View reviewed changes

electrum reviewed Sep 10, 2019

View reviewed changes

...to-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java Outdated Show resolved Hide resolved

electrum approved these changes Sep 10, 2019

View reviewed changes

lxynov added 3 commits September 10, 2019 16:32

Add a metastore method to list tables filtered on a property

6629585

Allow specifying metastore for Iceberg Connector

6d8576f

This is mainly used in tests.

Fix table listing in Iceberg

ac43013

Table listing in the Iceberg catalog should not include non-Iceberg tables.

lxynov force-pushed the iceberg-table-listing branch from 495770f to ac43013 Compare September 10, 2019 23:34

findepi merged commit fc52c6a into trinodb:master Sep 11, 2019

findepi mentioned this pull request Sep 11, 2019

Release notes for 319 #1378

Closed

6 tasks

lxynov deleted the iceberg-table-listing branch September 11, 2019 07:29

martint added this to the 319 milestone Sep 20, 2019

vrozov mentioned this pull request Sep 26, 2019

Spark-created Iceberg tables may be omitted in SHOW TABLES #1592

Closed

alexjo2144 mentioned this pull request Mar 22, 2022

TrinoHiveCatalog listTables should list all tables #11617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix table listing in Iceberg to skip non-Iceberg tables #1354

Fix table listing in Iceberg to skip non-Iceberg tables #1354

lxynov commented Aug 21, 2019

findepi Aug 22, 2019

findepi Aug 22, 2019

vrozov Sep 26, 2019

findepi Sep 26, 2019

vrozov Sep 26, 2019

lxynov commented Aug 23, 2019 •

edited

Loading

sopel39 left a comment

lxynov commented Aug 26, 2019

findepi left a comment

lxynov commented Aug 28, 2019

findepi left a comment

phd3 Aug 30, 2019

lxynov Aug 30, 2019

findepi Aug 30, 2019

phd3 Aug 30, 2019

phd3 Aug 30, 2019 •

edited

Loading

lxynov Sep 3, 2019 •

edited

Loading

electrum left a comment

lxynov commented Sep 3, 2019

lxynov commented Sep 10, 2019

findepi left a comment

electrum left a comment

findepi commented Sep 11, 2019

Fix table listing in Iceberg to skip non-Iceberg tables #1354

Fix table listing in Iceberg to skip non-Iceberg tables #1354

Conversation

lxynov commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxynov commented Aug 23, 2019 • edited Loading

sopel39 left a comment

Choose a reason for hiding this comment

lxynov commented Aug 26, 2019

findepi left a comment

Choose a reason for hiding this comment

lxynov commented Aug 28, 2019

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phd3 Aug 30, 2019 • edited Loading

Choose a reason for hiding this comment

lxynov Sep 3, 2019 • edited Loading

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

lxynov commented Sep 3, 2019

lxynov commented Sep 10, 2019

findepi left a comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

findepi commented Sep 11, 2019

lxynov commented Aug 23, 2019 •

edited

Loading

phd3 Aug 30, 2019 •

edited

Loading

lxynov Sep 3, 2019 •

edited

Loading